-
Notifications
You must be signed in to change notification settings - Fork 746
refactor(amazonq): separate edits from completion code path #7793
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
|
|
✅ I finished the code review, and didn't find any security or code quality issues. |
|
|
||
| if (position.isAfter(editor.selection.active)) { | ||
| // Edit suggestion works differently than completion suggestion, so even when it's a deletion and cause cursor to move back, we still allow the request to go through | ||
| if (position.isAfter(editor.selection.active) && items.length > 0 && !items[0].isInlineEdit) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (is completion) {
if (cursor moved before trigger point)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
donee
| ) | ||
| ps.push(editPromise) | ||
|
|
||
| let result = await Promise.race(ps) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
editPromise will likely always win the race because for most users the edit predictions are not enabled, hence its server API response will be very fast, and it will win the race.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
donee
| if (ps.length > 1 && result.items.length === 0) { | ||
| for (const p of ps) { | ||
| const r = await p | ||
| if (r.items.length > 0) { | ||
| result = r | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugly, but this is the best effort we can do atm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add another field suggestionType within InlineCompletionListWithReferences so that we know which suggestion type it is instead of this hack. Can follow up.
|
|
||
| const result = await this.getRecommendationsWithTimeout(languageClient, request, token) | ||
| // Best effort estimate of deletion | ||
| const isTriggerByDeletion = documentEventListener.isLastEventDeletion(document.uri.fsPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leigaol fyi, logic to handle trigger when users are deleting the code is moved here
Problem
related aws/language-servers#2058
Edits as a newly introduced feature, previously the implementation was living in the same flow as Completion. However the 2 suggestion types have few different product requirements, thus we decided to completely separate these 2 suggestions out in terms of their code path.
Key difference
Solution
onEditCompletionlanguage server API which is purely for Edits suggestion.feature/xbranches will not be squash-merged at release time.